TLS: Allow specification of both typed and non-typed san matchers in config by pradeepcrao · Pull Request #20529 · envoyproxy/envoy

pradeepcrao · 2022-03-25T15:57:17Z

Commit Message:
Additional Description:
Allow configs with both typed and non typed san matchers specified to allow config servers to use the same config for Envoys across multiple versions. The match_subject_alt_names field is ignored if match_typed_subject_alt_names is set.

Risk Level: Low
Testing: Modified test, ran //test/...
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

are provided in config. Signed-off-by: Pradeep Rao <pcrao@google.com>

Signed-off-by: Pradeep Rao <pcrao@google.com>

pradeepcrao · 2022-03-25T19:56:10Z

/assign adisuissa

suniltheta · 2022-03-26T06:41:25Z

Just curious if we can back port this to v1.21 ?

pradeepcrao · 2022-03-26T22:55:01Z

Just curious if we can back port this to v1.21 ?

Yes!

adisuissa

LGTM, thanks!

adisuissa · 2022-03-27T01:53:51Z

Assigning Greg who also reviewed the original PR (#18628).
/assign @ggreenway

soulxu · 2022-03-28T12:38:12Z

source/common/ssl/certificate_validation_context_config_impl.cc

      subject_alt_name_matchers(config.match_typed_subject_alt_names().begin(),
                                config.match_typed_subject_alt_names().end());
+  // If typed subject alt name matchers are provided in the config, don't check
+  // for the deprecated non-typed field.


This is minor behavior change we should add to release note I think?

Also worth to update the API doc https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/common.proto#extensions-transport-sockets-tls-v3-certificatevalidationcontext

Done. Could you please take a look?

soulxu · 2022-03-28T12:50:11Z

source/common/ssl/certificate_validation_context_config_impl.cc

    const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext& config) {
-  if (!config.match_typed_subject_alt_names().empty() &&
-      !config.match_subject_alt_names().empty()) {
-    throw EnvoyException("SAN-based verification using both match_typed_subject_alt_names and "


should we keep a warning message for the user? since we ignore this quietly, the user won't know the match_subject_alt_names was ignored.

Signed-off-by: Pradeep Rao <pcrao@google.com>

repokitteh-read-only · 2022-03-28T19:54:33Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20529 was synchronize by pradeepcrao.

see: more, trace.

soulxu · 2022-03-29T01:04:07Z

LGTM except the doc need to be fixed.

adisuissa · 2022-03-29T13:53:17Z

PTAL at CI
/wait

Signed-off-by: Pradeep Rao <pcrao@google.com>

pradeepcrao · 2022-03-29T18:35:13Z

The windows only failure (https://dev.azure.com/cncf/envoy/_build/results?buildId=104855&view=logs&j=b840a642-5ff3-5357-2e4b-e06e40b0cffd&t=67965174-5100-5631-9dc0-68b9f0aacb53&l=3085) seems to be unrelated to my changes. Will try retesting.

pradeepcrao · 2022-03-29T18:35:22Z

/retest

repokitteh-read-only · 2022-03-29T18:35:26Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20529 (comment) was created by @pradeepcrao.

see: more, trace.

soulxu · 2022-03-30T00:27:05Z

/retest

repokitteh-read-only · 2022-03-30T00:27:09Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20529 (comment) was created by @soulxu.

see: more, trace.

pradeepcrao · 2022-04-20T18:18:35Z

Is this situation unique, or are there other fields that are handled the same way?

@pradeepcrao any information about this question?

/wait-any

Hi Greg, this was the only one I could find:

envoy/api/envoy/config/core/v3/base.proto

Line 249 in f9d936e

    
           // If both :ref:`filter_metadata <envoy_v3_api_field_config.core.v3.Metadata.filter_metadata>`

(note that filter_metadata is not yet deprecated, but the plan is to deprecate it soon as per @yanjunxiang-google)

ggreenway · 2022-04-20T18:30:33Z

@envoyproxy/api-shepherds can you comment on if this the correct way to handle this from an API perspective? If there's nothing unique about this situation, how has this been handled for other similar issues?

/wait-any

Signed-off-by: Pradeep Rao <pcrao@google.com>

pradeepcrao · 2022-04-20T23:49:50Z

/retest

repokitteh-read-only · 2022-04-20T23:49:55Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20529 (comment) was created by @pradeepcrao.

see: more, trace.

htuch · 2022-04-21T05:13:32Z

@ggreenway I think this is a common pattern (introduce new field which is effectively oneof, prefer one over the other), but there are also places where you see both fields continuing to be used, e.g. verify_certificate_{spki,hash}. I think as long as you can set both, and prefer the most correct/safest/etc, you enable safe rollouts.

ggreenway

Code looks good; release notes have issues.

/wait

ggreenway · 2022-04-21T15:08:08Z

docs/root/version_history/current.rst

 ----------------------
 *Changes that may cause incompatibilities for some users, but should not for most*

+* access_log: log all header values in the grpc access log.


Something went wrong here; I think you picked up a bunch of entries during a bad merge or something.

ggreenway · 2022-04-21T15:12:05Z

docs/root/version_history/current.rst

+* sip-proxy: add customized affinity support by adding :ref:`tra_service_config <envoy_v3_api_msg_extensions.filters.network.sip_proxy.tra.v3alpha.TraServiceConfig>` and :ref:`customized_affinity <envoy_v3_api_msg_extensions.filters.network.sip_proxy.v3alpha.CustomizedAffinity>`.
+* sip-proxy: add support for the ``503`` response code. When there is something wrong occurred, send ``503 Service Unavailable`` back to downstream.
+* stateful session http filter: only enable cookie based session state when request path matches the configured cookie path.
+* tls: if both :ref:`match_subject_alt_names <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_subject_alt_names>` and :ref:`match_typed_subject_alt_names <envoy_v3_api_field_extensions.transport_sockets.tls.v3.CertificateValidationContext.match_typed_subject_alt_names>` are specified, the former (deprecated) field is ignored.


Please note the previous behavior (that this was an error).

Done, PTAL.

New text looks good, but you're still adding 50 entries to the release notes. Please fix.

Sorry, missed that. Must've have happened because of the 1.22 release. Done now.

Signed-off-by: Pradeep Rao <pcrao@google.com>

phlax · 2022-04-25T08:58:48Z

/retest

repokitteh-read-only · 2022-04-25T08:58:52Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #20529 (comment) was created by @phlax.

see: more, trace.

adisuissa

LGTM, thanks!
/lgtm api

pradeepcrao · 2022-04-27T17:18:23Z

@ggreenway Do I have your approval to backport this to 1.21 and 1.22?

…config (#21165) Backport of #20529 Signed-off-by: Pradeep Rao <pcrao@google.com>

…config (#21170) Backport of #20529 Signed-off-by: Pradeep Rao <pcrao@google.com>

…config (envoyproxy#20529) Allow configs with both typed and non typed san matchers specified to allow config servers to use the same config for Envoys across multiple versions. The match_subject_alt_names field is ignored if match_typed_subject_alt_names is set. Signed-off-by: Pradeep Rao <pcrao@google.com>

pradeepcrao added 2 commits March 25, 2022 15:42

Ignore non typed san matchers when both typed and non typed san matchers

85a2cea

are provided in config. Signed-off-by: Pradeep Rao <pcrao@google.com>

Merge remote-tracking branch 'upstream/main' into san_backwards

edbe61f

Signed-off-by: Pradeep Rao <pcrao@google.com>

pradeepcrao requested review from ggreenway and lizan as code owners March 25, 2022 15:57

repokitteh-read-only bot assigned adisuissa Mar 25, 2022

adisuissa previously approved these changes Mar 27, 2022

View reviewed changes

repokitteh-read-only bot assigned ggreenway Mar 27, 2022

soulxu reviewed Mar 28, 2022

View reviewed changes

pradeepcrao added 2 commits March 28, 2022 19:53

Address feedback.

c3300cc

Signed-off-by: Pradeep Rao <pcrao@google.com>

Merge remote-tracking branch 'upstream/main' into san_backwards

326ad25

Signed-off-by: Pradeep Rao <pcrao@google.com>

pradeepcrao dismissed adisuissa’s stale review via 326ad25 March 28, 2022 19:54

repokitteh-read-only bot added the api label Mar 28, 2022

repokitteh-read-only bot added the waiting label Mar 29, 2022

pradeepcrao added 2 commits March 29, 2022 14:11

Fix doc build failure.

9ef7193

Signed-off-by: Pradeep Rao <pcrao@google.com>

Merge remote-tracking branch 'upstream/main' into san_backwards

da2b461

Signed-off-by: Pradeep Rao <pcrao@google.com>

repokitteh-read-only bot removed the waiting label Mar 29, 2022

Fix format.

a47153f

Signed-off-by: Pradeep Rao <pcrao@google.com>

repokitteh-read-only bot removed the waiting:any label Apr 20, 2022

repokitteh-read-only bot added the waiting:any label Apr 20, 2022

Fix format.

1e5c762

Signed-off-by: Pradeep Rao <pcrao@google.com>

repokitteh-read-only bot removed the waiting:any label Apr 20, 2022

ggreenway requested changes Apr 21, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 21, 2022

ggreenway reviewed Apr 21, 2022

View reviewed changes

Modify doc.

508fc0c

Signed-off-by: Pradeep Rao <pcrao@google.com>

repokitteh-read-only bot removed the waiting label Apr 21, 2022

Fix bad merge

c1177fe

Signed-off-by: Pradeep Rao <pcrao@google.com>

ggreenway approved these changes Apr 21, 2022

View reviewed changes

adisuissa approved these changes Apr 25, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Apr 25, 2022

ggreenway merged commit ef08b1c into envoyproxy:main Apr 25, 2022

ggreenway pushed a commit that referenced this pull request May 16, 2022

TLS: Allow specification of both typed and non-typed san matchers in …

e7ba5cc

…config (#21165) Backport of #20529 Signed-off-by: Pradeep Rao <pcrao@google.com>

ggreenway pushed a commit that referenced this pull request May 16, 2022

TLS: Allow specification of both typed and non-typed san matchers in …

11d1a0f

…config (#21170) Backport of #20529 Signed-off-by: Pradeep Rao <pcrao@google.com>

phlax removed the backport/review Request to backport to stable releases label Feb 1, 2023

Conversation

pradeepcrao commented Mar 25, 2022

Uh oh!

pradeepcrao commented Mar 25, 2022

Uh oh!

suniltheta commented Mar 26, 2022

Uh oh!

pradeepcrao commented Mar 26, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Mar 27, 2022

Uh oh!

soulxu Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Mar 28, 2022

Uh oh!

soulxu commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adisuissa commented Mar 29, 2022

Uh oh!

pradeepcrao commented Mar 29, 2022

Uh oh!

pradeepcrao commented Mar 29, 2022

Uh oh!

repokitteh-read-only bot commented Mar 29, 2022

Uh oh!

soulxu commented Mar 30, 2022

Uh oh!

repokitteh-read-only bot commented Mar 30, 2022

Uh oh!

pradeepcrao commented Apr 20, 2022

Uh oh!

ggreenway commented Apr 20, 2022

Uh oh!

pradeepcrao commented Apr 20, 2022

Uh oh!

repokitteh-read-only bot commented Apr 20, 2022

Uh oh!

htuch commented Apr 21, 2022

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlax commented Apr 25, 2022

Uh oh!

repokitteh-read-only bot commented Apr 25, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

pradeepcrao commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

soulxu Mar 28, 2022 •

edited

Loading

soulxu commented Mar 29, 2022 •

edited

Loading